Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LLVM update 43d71ba #3086

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from
Draft

Conversation

brnorris03
Copy link
Collaborator

@brnorris03 brnorris03 commented Feb 21, 2025

@brnorris03 brnorris03 changed the title Llvm update 0e779ad LLVM update 0e779ad Feb 21, 2025
@brnorris03
Copy link
Collaborator Author

@AlexandreEichenberger -- this seems like a rather large update, so I could use some help. There were a lot of TOSA dialect changes, and I am going through lit tests now and updating them (no problems there so far) -- should be done today hopefully. The check-onnx-numerical also passes.

However, most of the backend tests fail, e.g., in the check-onnx-backend-constant, and I am not sure what the best way is to investigate these failures.

Signed-off-by: Boyana Norris <[email protected]>
Signed-off-by: Boyana Norris <[email protected]>
Signed-off-by: Boyana Norris <[email protected]>
Signed-off-by: Boyana Norris <[email protected]>
Signed-off-by: Boyana Norris <[email protected]>
Signed-off-by: Boyana Norris <[email protected]>

auto int8Type = rewriter().getI8Type();
auto shiftValue = TosaBuilder::createConst(
ArrayRef<int8_t>{static_cast<int8_t>(shift)}, {1}, int8Type);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO its better to change the TosaBuilder::mul parameter shift to int8_t isntead of having this hidden cast inside the function

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate a bit more what you mean by "change the TosaBuilder::mul parameter shift" -- change it where?

Copy link
Collaborator

@jorickert jorickert Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the function signature: Value TosaBuilder::mul(Value &lhs, Value &rhs, int32_t shift).
Right now it takes an int32_t paramter and internally cast it to int8_t.
I would prefer it to change the function signature to Value TosaBuilder::mul(Value &lhs, Value &rhs, int8_t shift) so that it is clear for callers that it only supports int8 shift values

@AlexandreEichenberger
Copy link
Collaborator

@jorickert thanks for helping with the TOSA related updates.
@brnorris03 if you feel that there are issues that are not TOSA related and you need further help, I am back at work and would be happy to help.

@brnorris03
Copy link
Collaborator Author

@jorickert thanks for helping with the TOSA related updates. @brnorris03 if you feel that there are issues that are not TOSA related and you need further help, I am back at work and would be happy to help.

Thanks! I did have everything passing locally, but the stablehlo build failed CI. So I updated the llvm and stablehlo versions again, which brought in another pile of TOSA changes; everything builds but I have not had a chance to update all the lit tests yet.

@brnorris03 brnorris03 changed the title LLVM update 0e779ad LLVM update 43d71ba Mar 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants